Use POSIX separators in file-protocol storage URLs and paths by lochhh · Pull Request #1521 · datajoint/datajoint-python

lochhh · 2026-07-23T14:08:01Z

StorageBackend._full_path() returned OS-native separators for the file protocol (via bare str(Path(...))), so paths came out with backslashes on Windows. This silently broke delete_schema_path's empty parent dir pruning, which silently no-ops on backslash paths:

datajoint-python/src/datajoint/gc.py

Lines 293 to 294 in afc350f

    
           root = self.backend._full_path("").rstrip("/") 
        
           parent = full_path.rsplit("/", 1)[0]

This PR

fixes _full_path() such that it now returns /-separated paths for the file protocol
additionally cleans up normalize_to_url() and get_url() which already handled Windows
paths correctly, but did so via a duplicated ad-hoc .replace(chr(92), '/'), by extracting a shared _path_to_file_url() helper (.as_posix() + lstrip) to match the .as_posix() idiom in _full_path()
adds the following tests:
- Platform-independent coverage via PureWindowsPath/PurePosixPath for _full_path and _path_to_file_url
- Real tmp_path wiring test for get_url under the file protocol
- Regression test for delete_schema_path parent-dir pruning

…(backslash)

dimitri-yatsenko

Nice catch, and the right fix. .as_posix() on the file branch of _full_path also completes a convention already used elsewhere in this file — get_url and normalize_to_url both force forward slashes for Windows via .replace(chr(92), '/'), and _full_path's file branch was the one spot still emitting native separators. Every file-protocol operation routes through _full_path (put_file/get_file/put/get_buffer/exists/isdir/remove/size/open/put_folder/get_url), so this single change covers all consumers, and fsspec's LocalFileSystem accepts forward-slash (incl. drive-letter) paths on Windows — no risk to the read/write paths. The PureWindowsPath monkeypatch makes the test platform-independent (runs on the Linux CI) and it fails pre-fix — nice.

LGTM to merge. One thing worth confirming, and one FYI:

Confirm the end-to-end GC path (not blocking, but worth a line). The new test asserts the _full_path separator contract, but it doesn't exercise the actual misclassification (list_hash_paths/list_schema_paths → _is_covered). That matters because the CI matrix has no Windows runner, and on Linux .as_posix() == str(...), so nothing automated exercises the failing path. Also, #1520 describes the mechanism as file_path.replace(full_root, ""), but the code on master uses a length slice — file_path[len(full_root):].lstrip("/") (gc.py:212, 261) — which is largely insensitive to separator style. So it'd be good to either add a small integration test (walk a file store with a Windows-style _full_path and assert the relative paths match a reference set) or confirm the manual Win11 collect() repro now passes end-to-end — just so we're sure separators are the true root cause rather than, say, a relative-vs-absolute prefix mismatch.

FYI: added the bug label and the v2.3.3 milestone so this data-loss fix lands in the release-drafter notes.

Nit (non-blocking): this file now has two idioms for the same thing — .as_posix() (new) vs str(...).replace(chr(92), '/') (lines 83, 462). .as_posix() is cleaner; worth unifying in a follow-up.

dimitri-yatsenko · 2026-07-23T16:23:23Z

Opened #1522 to add a Windows unit-test leg to CI (windows-latest, container-free tests/unit on Python 3.10 and 3.14). The matrix was Linux-only, so this class of OS-specific bug — path separators here — had no automated guard; #1522 gives us the runner so this regression stays caught. It uses pip (.[test]) rather than pixi, since pixi targets linux/osx only.

That addresses the CI half of the coverage note above. The end-to-end GC integration test (walking a file store and asserting relative paths match references) can now land on top of that runner as a follow-up.

lochhh · 2026-07-23T17:15:46Z

I've closed #1520 (comment).
But switching to as_posix fixes a separate minor issue: delete_schema_path's empty parent dir pruning silently no-ops on backslash paths, skipping cleanup.

datajoint-python/src/datajoint/gc.py

Lines 293 to 294 in afc350f

    
           root = self.backend._full_path("").rstrip("/") 
        
           parent = full_path.rsplit("/", 1)[0]

I've updated the PR title and description to reflect this.

dimitri-yatsenko · 2026-07-23T17:38:29Z

Re-reviewed after the reframe (the #1520 GC-misclassification rationale withdrawn, PR now justified by delete_schema_path's empty-parent pruning). Verdict unchanged — approve. I traced the code and ran the test.

The reframed bug is real. gc.py's prune loop (293-302) walks up empty parent dirs after deleting an orphan. Pre-fix on a Windows-style path:

full_path = _full_path(path) → data\blobs\…\field_token (backslashes)
parent = full_path.rsplit("/", 1)[0] → no / to split on, so parent is the whole file path, not its directory
fs.ls(parent) on a file returns that file (non-empty) → break → pruning silently no-ops; empty dirs leak

.as_posix() gives data/blobs/…, so rsplit("/") yields the real parent and the walk-up works. Confirmed.

This also retroactively fits the #1520 withdrawal: list_*_paths strips the root with a length slice (file_path[len(full_root):]), which is insensitive to separator style — so separators were never the #1520 root cause.

No regression. Every file-protocol consumer of _full_path (objectref, npy, staged_insert, all storage.py ops) routes through fsspec's LocalFileSystem, which accepts forward-slash paths on Windows; on Linux .as_posix() == str(...), byte-identical. get_url re-resolves via Path(full_path).resolve() independently, so it doesn't double-handle.

Test. Passes on the branch; I confirmed it fails pre-fix (str(...) → data\blobs\…), so it's a valid regression guard, and the PureWindowsPath monkeypatch makes it run on the Linux CI.

One open item (non-blocking): the test asserts only the _full_path unit contract — it doesn't exercise delete_schema_path's pruning, which is now the PR's sole justification, and the matrix has no Windows runner yet. Covered by the plan (#1522 Windows leg + GC integration test as follow-up), so fine to merge — but a cheap interim guard would be a unit test that calls delete_schema_path against a fake fs with a monkeypatched Windows-style _full_path and asserts rmdir is invoked on the parent dir.

Minor: two forward-slash idioms now coexist — .as_posix() (new) vs .replace(chr(92), '/') at storage.py:83,462. Those operate on abspath strings, not Path objects, so they can't trivially adopt .as_posix(); worth unifying in a follow-up.

- Add shared _path_to_file_url helper - Add platform-independent unit tests for _path_to_file_url helper - Add real tmp_path wiring test for get_url

lochhh · 2026-07-24T12:22:59Z

Thanks. I've updated the PR title and description to additionally cover the below.

One open item (non-blocking): the test asserts only the _full_path unit contract — it doesn't exercise delete_schema_path's pruning, which is now the PR's sole justification, and the matrix has no Windows runner yet. Covered by the plan (#1522 Windows leg + GC integration test as follow-up), so fine to merge — but a cheap interim guard would be a unit test that calls delete_schema_path against a fake fs with a monkeypatched Windows-style _full_path and asserts rmdir is invoked on the parent dir.

Test delete_schema_path parent dir pruning

Minor: two forward-slash idioms now coexist — .as_posix() (new) vs .replace(chr(92), '/') at storage.py:83,462. Those operate on abspath strings, not Path objects, so they can't trivially adopt .as_posix(); worth unifying in a follow-up.

Unify file:// URL slash handling for Windows and POSIX paths

lochhh added 3 commits July 23, 2026 14:51

Test StorageBackend._full_path with with native Windows separators …

03436ff

…(backslash)

Fix Windows backslash bug in file-protocol storage paths

9e8f675

Make _full_path test platform-independent

db07c10

dimitri-yatsenko added the bug Indicates an unexpected problem or unintended behavior label Jul 23, 2026

dimitri-yatsenko added this to the v2.3.3 milestone Jul 23, 2026

dimitri-yatsenko reviewed Jul 23, 2026

View reviewed changes

dimitri-yatsenko mentioned this pull request Jul 23, 2026

ci: add Windows unit-test job to catch OS-specific path bugs #1522

Open

lochhh changed the title ~~Fix backslash/forward-slash path mismatch in file-protocol stores~~ Use POSIX separators in file-protocol paths on Windows Jul 23, 2026

lochhh changed the title ~~Use POSIX separators in file-protocol paths on Windows~~ Use POSIX separators in file-protocol paths Jul 23, 2026

lochhh marked this pull request as ready for review July 23, 2026 17:25

lochhh added 2 commits July 24, 2026 10:45

Test delete_schema_path parent dir pruning

67df781

Unify file:// URL slash handling for Windows and POSIX paths

1852c1a

- Add shared _path_to_file_url helper - Add platform-independent unit tests for _path_to_file_url helper - Add real tmp_path wiring test for get_url

lochhh changed the title ~~Use POSIX separators in file-protocol paths~~ Use POSIX separators in file-protocol storage URLs and paths Jul 24, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use POSIX separators in file-protocol storage URLs and paths#1521

Use POSIX separators in file-protocol storage URLs and paths#1521
lochhh wants to merge 5 commits into
datajoint:masterfrom
lochhh:fix-storage-win

lochhh commented Jul 23, 2026 •

edited

Loading

Uh oh!

dimitri-yatsenko left a comment

Uh oh!

dimitri-yatsenko commented Jul 23, 2026

Uh oh!

lochhh commented Jul 23, 2026 •

edited

Loading

Uh oh!

dimitri-yatsenko commented Jul 23, 2026

Uh oh!

lochhh commented Jul 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	root = self.backend._full_path("").rstrip("/")
	parent = full_path.rsplit("/", 1)[0]

Uh oh!

Conversation

lochhh commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimitri-yatsenko left a comment

Choose a reason for hiding this comment

Uh oh!

dimitri-yatsenko commented Jul 23, 2026

Uh oh!

lochhh commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimitri-yatsenko commented Jul 23, 2026

Uh oh!

lochhh commented Jul 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lochhh commented Jul 23, 2026 •

edited

Loading

lochhh commented Jul 23, 2026 •

edited

Loading